Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Ellipsoids3D archetype. #6853

Merged
merged 22 commits into from
Jul 15, 2024
Merged

Add Ellipsoids3D archetype. #6853

merged 22 commits into from
Jul 15, 2024

Conversation

kpreid
Copy link
Collaborator

@kpreid kpreid commented Jul 11, 2024

What

Implement an Ellipsoids archetype and visualizer. This may be used to render ellipsoids, or spheres where Points3D is unsuitable because the actual boundary in space is significant. It is intended as a first step towards completing #1361, by adding ellipsoids and by sketching out a general mechanism for rendering procedurally-generated shapes, which will be reusable for cylinders and anything which needs many vertices to render.

The archetype, extensions, etc. are largely forked off of Boxes3d.

Unfinished parts / future work:

  • Currently, the spheres are always drawn as wireframes (with an automatically chosen mesh subdivision level). We will want to also support solid rendering (i.e. triangles in place of lines), but many use cases for spheres might want them to be transparent, which isn't yet properly supported in 3D space views (Draw order & (order independent) transparency #702), so I chose wireframe as the first version.

  • Currently, all of the wireframe lines are piped through the LineDrawableBuilder every frame. Instead, they should be drawn as instanced meshes; that will require implementing a new renderer that invokes the existing line shader with the different data layout. However, this current implementation should not be worse than if the user were to create a similar number of lines directly.

  • The sphere mesh needs a choice of subdivision level. Currently, it is automatically derived from the size of the sphere in the scene, which may be inappropriate depending on the scale of scene units or the specific application. I'd like feedback on the best way to handle this. Just add another component, perhaps kind of like the Radius component?

  • I‘m not sure what the best way to handle the (future) errors in generating meshes is. Should it return a SpaceViewSystemExecutionError (which hides unrelated data), just draw nothing and continue (which could lead to silent missing data), or something else?

rerun_example_ellipsoid_batch

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

Currently, the visualizer is inefficient, in that it pipes everything
through the `LineDrawableBuilder` every frame unnecessarily. However,
this should not be worse than if the user were to create a similar
number of lines directly.
@kpreid kpreid added 🍏 primitives Relating to Rerun primitives include in changelog labels Jul 11, 2024
Cargo.lock Outdated Show resolved Hide resolved
@emilk
Copy link
Member

emilk commented Jul 11, 2024

Needs screenshot and instructions on how to test!

@kpreid
Copy link
Collaborator Author

kpreid commented Jul 11, 2024

Needs screenshot and instructions on how to test!

I've added a screenshot to the PR description. One is still needed in the archetype docs, but I assume I don't actually have permissions to run upload_image.py to create one. Also, the comments there suggest a 16:9 aspect ratio, but the example I created looks a little silly that way, and the existing archetype doc screenshots aren't 16:9 — should I stick to that, or do what looks good?

For testing, cargo run --bin snippets -- ellipsoid_batch should do.

@kpreid
Copy link
Collaborator Author

kpreid commented Jul 11, 2024

What pull request labels are appropriate to satisfy the labels check? Should it be labeled with all three APIs since it touches all of them? What about the rendering aspect? Is that re_viewer since that's the closest dependent of re_space_view_spatial?

@kpreid kpreid closed this Jul 11, 2024
@kpreid kpreid reopened this Jul 11, 2024
@emilk emilk added 📺 re_viewer affects re_viewer itself 🪵 Log & send APIs Affects the user-facing API for all languages labels Jul 12, 2024
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me!

rerun_cpp/src/rerun/archetypes/ellipsoids_ext.cpp Outdated Show resolved Hide resolved
rerun_cpp/src/rerun/archetypes/ellipsoids_ext.cpp Outdated Show resolved Hide resolved
rerun_cpp/src/rerun/archetypes/ellipsoids_ext.cpp Outdated Show resolved Hide resolved
@emilk
Copy link
Member

emilk commented Jul 14, 2024

The snippets test is failing:

> pixi run -e py docs/snippets/compare_snippet_output.py archetypes/ellipsoid_batch
…
Traceback (most recent call last):
  File "/Users/emilk/code/rerun/rerun/docs/snippets/all/archetypes/ellipsoid_batch.py", line 12, in <module>
    rr.Ellipsoids(
    ^^^^^^^^^^^^^
AttributeError: module 'rerun' has no attribute 'Ellipsoids'

EDIT: fixed.

@kpreid
Copy link
Collaborator Author

kpreid commented Jul 15, 2024

The remaining unchecked checkbox says to test in the web viewer, but following the links says “pr/6853 is not a valid web viewer version”. Should I continue regardless? Does that need to be authorized?

@emilk
Copy link
Member

emilk commented Jul 15, 2024

Sadly our CI for contributors doesn't build a web viewer. You can test it locally with pixi run rerun-web… but it should be fine. Let's merge this and fix anything that pops up later :)

@emilk emilk merged commit 82a47b9 into rerun-io:main Jul 15, 2024
29 of 31 checks passed
emilk pushed a commit that referenced this pull request Jul 15, 2024
### What

Should fix link checker error in CI. Introduced in #6853 

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/{{pr.number}}?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/{{pr.number}}?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/{{pr.number}})
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
@teh-cmc teh-cmc changed the title Add Ellipsoids archetype. Add Ellipsoids3d archetype. Aug 14, 2024
@teh-cmc teh-cmc changed the title Add Ellipsoids3d archetype. Add Ellipsoids3D archetype. Aug 14, 2024
@kpreid kpreid deleted the ellipsoids branch October 2, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages 🍏 primitives Relating to Rerun primitives 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants